Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

Requesting changes for four P1 blockers on head 6fc7078b7c01df729b9a08d24a25220de92cda2f:

1. The new aggregate IDs can be sent to previous-version CNs during a rolling upgrade without a capability gate.
2. Exact percentile state is unbounded for H0 and a single hot group, so normal large inputs can OOM despite SpillMem.
3. Making WITHIN reserved breaks SQL and schemas that previously used it as an unquoted identifier.
4. PERCENTILE_CONT declares an invalid/insufficient decimal result type for valid maximum-precision inputs and can overflow even at p=0.

I reviewed the full diff from merge-base a4b0ce286d182c24efd5a349620ae37016262301, including parser → binder → plan → compile → remote aggregate → merge/spill/flush/free. All 26 GitHub checks are complete with no failing conclusion. The added parser tests pass locally; a base/head parser probe also confirms the WITHIN compatibility regression. No P2/P3-only items are being used to block this review.

1. 远程执行能力门控

• 新增 ordered-set percentile 聚合 ID 的 MORPC 协议门控。
• merge main 后，保留 main 的 MORPCVersion11/12/13，将 percentile 能力顺延为 MORPCVersion14。
• remote aggregate / merge group 在协议版本不足时拒绝远程执行，避免新 CN 把 PERCENTILE_CONT/DISC 发给旧 CN 后报 unexpected aggID。

2. percentile 聚合内存无限增长

• 为 ordered percentile 增加聚合器内部 spill 机制。
• 覆盖 H0 聚合和单热点 group 场景，避免只依赖 generic group spill。
• 补充了 H0、大单组、generic spill 组合场景的回归测试。

3. WITHIN 关键字兼容性

• 将 WITHIN 从保留关键字改为上下文关键字/非保留用法。
• 保证已有 SQL 如 select within from t、create table t (within int) 不被破坏。
• 补充 parser 兼容性测试。

4. decimal 返回类型/溢出

• 修正 PERCENTILE_CONT 对 DECIMAL 输入的返回类型推导，避免生成非法 DECIMAL(38,39)。
• 处理最大 precision/scale 边界，避免 p=0 时仍因内部放大计算溢出。
• 补充 DECIMAL(38,38) 和 38 位整数边界测试。

5. GROUP_CONCAT 双 ORDER BY

• 对 group_concat(x order by y) within group (order by z) 这类双排序写法改为拒绝。
• 避免静默丢弃其中一个 ORDER BY 导致语义被悄悄改写。
• 补充 parser 覆盖。

Reviewed exact head 6fc7078 after the successful CI rollup.

[P1] pkg/sql/colexec/aggexec/ordered_percentile.go:49: PERCENTILE_CONT always declares decimal128 scale input scale plus one. A valid DECIMAL(38,38) input therefore becomes the invalid DECIMAL(38,39), and flush multiplies the 38-scale unscaled value by 10; values such as 0.9 overflow even for percentile 0, which should simply return the input. Cap or otherwise choose a representable result precision/scale and test the maximum-scale boundary.

[P2] pkg/sql/parsers/dialect/mysql/mysql_sql.y:12524: GROUP_CONCAT accepts both its in-parentheses ORDER BY and WITHIN GROUP ORDER BY, then silently discards the first list in favor of the second. Reject the ambiguous double-order form instead of changing its meaning silently, and add parser coverage.

1. 远程执行能力门控

• 新增 ordered-set percentile 聚合 ID 的 MORPC 协议门控。
• merge main 后，保留 main 的 MORPCVersion11/12/13，将 percentile 能力顺延为 MORPCVersion14。
• remote aggregate / merge group 在协议版本不足时拒绝远程执行，避免新 CN 把 PERCENTILE_CONT/DISC 发给旧 CN 后报 unexpected aggID。

2. percentile 聚合内存无限增长

• 为 ordered percentile 增加聚合器内部 spill 机制。
• 覆盖 H0 聚合和单热点 group 场景，避免只依赖 generic group spill。
• 补充了 H0、大单组、generic spill 组合场景的回归测试。

3. WITHIN 关键字兼容性

• 将 WITHIN 从保留关键字改为上下文关键字/非保留用法。
• 保证已有 SQL 如 select within from t、create table t (within int) 不被破坏。
• 补充 parser 兼容性测试。

4. decimal 返回类型/溢出

• 修正 PERCENTILE_CONT 对 DECIMAL 输入的返回类型推导，避免生成非法 DECIMAL(38,39)。
• 处理最大 precision/scale 边界，避免 p=0 时仍因内部放大计算溢出。
• 补充 DECIMAL(38,38) 和 38 位整数边界测试。

5. GROUP_CONCAT 双 ORDER BY

• 对 group_concat(x order by y) within group (order by z) 这类双排序写法改为拒绝。
• 避免静默丢弃其中一个 ORDER BY 导致语义被悄悄改写。
• 补充 parser 覆盖。

Requesting changes on exact head 15d9b16e832431baa3cdedc892813aab2ded3892 for two P1 blockers:

1. Maximum-width DECIMAL inputs can still make PERCENTILE_CONT return a discrete rounded value instead of the required continuous interpolation.
2. The contextual WITHIN lexer still breaks existing identifier use when the next token is GROUP.

I read the previous review, all review threads, and the PR conversation. The author replies explicitly address the previous findings, and the protocol gate, bounded spill path, basic identifier cases, and double-ORDER rejection are present; the two blockers below remain on the current head.

Review scope: full diff from merge-base 3c7392a0f86b23712da71f31d7a9198a2c8591a9, including parser → binder → plan/type inference → compile/remote protocol → aggregate merge/spill/flush/free and tests. All 26 GitHub checks are complete with no failing conclusion. Local targeted tests could not start in the isolated worktree because the required cgo/libmo.dylib artifact is absent; that is a test prerequisite failure, not test evidence. Both blockers below are directly provable from the current code paths.

1. 远程执行能力门控

• ordered-set percentile 新增独立 MORPC 能力门控。
• merge main 后，main 已占用 MORPCVersion14 给 utf8mb4 text MIN/MAX collation semantics，所以 ordered-set percentile 顺延为 MORPCVersion15。
• 远程执行时：
  • PERCENTILE_CONT/DISC 要求 v15；
  • GROUP_CONCAT ORDER BY 继续要求 v6；
  • collation-aware text MIN/MAX 继续要求 v14。

2. DECIMAL(38,*) 连续插值问题

• PERCENTILE_CONT 对最大宽度 DECIMAL(38,*) order 表达式改为拒绝，避免 0,1,p=0.5 被错误四舍五入成离散值。
• PERCENTILE_DISC 不受影响，仍支持 DECIMAL(38,*)。
• 增加 UT 和 BVT：
  • PERCENTILE_CONT + DECIMAL(38,0) 拒绝；
  • PERCENTILE_DISC + DECIMAL(38,0) 正常返回；
  • DECIMAL(37,0) 连续插值返回 0.5。

3. WITHIN 关键字兼容性

• scanner 只在完整识别到 WITHIN GROUP ( 时返回 WITHIN token。
• WITHIN /* comment */ GROUP (...) 支持。
• SELECT 1 AS within GROUP BY 1 继续把 within 当普通 identifier。
• 增加 parser 回归测试覆盖 alias 邻接 GROUP BY 和注释场景。

4. GROUP_CONCAT 双 ORDER BY

• group_concat(v order by x) within group (order by y) 改为报错。
• 避免静默丢弃其中一个排序条件造成语义变化。

5. bounded spill / 大 group 内存问题

• ordered percentile 增加聚合器内部 spill 逻辑。
• 覆盖 H0、大单组、generic spill 组合场景，避免只依赖 group spill 导致 OOM。

Reviewed exact head 902c821e4b5b63984df668ebb256f1e19c91275b against current origin/main 938a4dddd41298f53a067ae7ba6aef36b443564d (merge-base d01c85974f9540f8baa2e768ca86fea952941fe2). No P0/P1 blocker remains; one new P2 correctness issue is recorded inline, so this review is COMMENT rather than REQUEST_CHANGES.

I read the previous reviews, all review threads, and the PR conversation. The author reply after the prior review explicitly addresses both earlier P1s, and the current code/tests now reject PERCENTILE_CONT on DECIMAL(38,*), retain PERCENTILE_DISC support, and recognize only the full WITHIN GROUP ( phrase (including comments), preserving identifier use. Those prior blockers are resolved.

All 26 GitHub checks are complete with no failing conclusion, and git diff --check passes. Targeted local Go tests could not start in the isolated PR worktree because the required cgo/libmo.dylib artifact is absent; this is a prerequisite failure, not test evidence.

1. 修复 FLOAT/DOUBLE NaN 在 percentile spill 下结果不稳定的问题

• 原因：spill run 创建时用 mosort.SortByVectors，FLOAT NaN 没有 total order；但 k-way merge 用 cmp.Compare，NaN 有 total order，导致 spill/non-spill 可能选到不同 rank。
• 修改：sortOrderedPercentileValues 改为直接使用 compareOrderedPercentileValue 排序。
• 结果：in-memory sort、spill run creation、k-way merge 三处使用同一套 ASC/DESC comparator。

2. 补充 NaN spill 等价测试

• 新增 FLOAT32 / FLOAT64 覆盖。
• 覆盖 ASC / DESC。
• 覆盖 PERCENTILE_CONT / PERCENTILE_DISC。
• 对比 spill 与 non-spill 结果一致。
• 额外断言 NaN 在 ASC/DESC 下的排序位置。

3. merge main 后修正 MORPC 协议版本冲突

• upstream main 已把 MORPCVersion16 用于 information_schema CHECK_CONSTRAINTS。
• percentile aggregate IDs 改为使用新的 MORPCVersion17。
• 远程执行门控改为 >= MORPCVersion17。
• 测试确认：Version16 仍拒绝 percentile remote，Version17 才允许。

4. 更新冲突 result 文件

• mo_cloud.result 接受 upstream 新增的 information_schema.check_constraints。
• information_schema 表数量从 24 改为 25。

Deep re-reviewed exact head 902c821e4b5b63984df668ebb256f1e19c91275b, including all existing reviews, inline comments, and author replies. The earlier protocol-version, bounded-spill, DECIMAL(38,*), contextual WITHIN, and double-ORDER findings are closed. One result-correctness blocker remains.

sortOrderedPercentileValues creates FLOAT32/FLOAT64 runs through mosort.SortByVectors, whose float ordering uses ordinary </> and therefore does not total-order NaN. The external merge heap uses cmp.Compare, which does total-order NaN. The merge consequently assumes a different ordering from the ordering used to create each run.

I reproduced this through the aggregate's public BulkFill/spill/Flush path with 20,001 FLOAT64 values and NaN at the first row of the second 10k batch: PERCENTILE_DISC(0) ascending returns 0 without spill but NaN after the 64-KiB spill boundary. The result therefore depends on input batching/memory pressure. Please use one explicit total comparator for both in-memory run sorting and merge (with defined ASC/DESC NaN semantics), and add spill/non-spill equivalence coverage for FLOAT32/FLOAT64 and CONT/DISC.

Focused parser, aggregate, group-spill, compile, and remote-protocol tests otherwise pass under controlled CGo; required CI is green and git diff --check passes.

1. 修复 FLOAT/DOUBLE NaN 在 percentile spill 下结果不稳定的问题

• 原因：spill run 创建时用 mosort.SortByVectors，FLOAT NaN 没有 total order；但 k-way merge 用 cmp.Compare，NaN 有 total order，导致 spill/non-spill 可能选到不同 rank。
• 修改：sortOrderedPercentileValues 改为直接使用 compareOrderedPercentileValue 排序。
• 结果：in-memory sort、spill run creation、k-way merge 三处使用同一套 ASC/DESC comparator。

2. 补充 NaN spill 等价测试

• 新增 FLOAT32 / FLOAT64 覆盖。
• 覆盖 ASC / DESC。
• 覆盖 PERCENTILE_CONT / PERCENTILE_DISC。
• 对比 spill 与 non-spill 结果一致。
• 额外断言 NaN 在 ASC/DESC 下的排序位置。

3. merge main 后修正 MORPC 协议版本冲突

• upstream main 已把 MORPCVersion16 用于 information_schema CHECK_CONSTRAINTS。
• percentile aggregate IDs 改为使用新的 MORPCVersion17。
• 远程执行门控改为 >= MORPCVersion17。
• 测试确认：Version16 仍拒绝 percentile remote，Version17 才允许。

4. 更新冲突 result 文件

• mo_cloud.result 接受 upstream 新增的 information_schema.check_constraints。
• information_schema 表数量从 24 改为 25。

Re-reviewed exact head 902c821e4b5b63984df668ebb256f1e19c91275b; all checks are terminal and successful. The earlier protocol, spill-bounding, parser compatibility, decimal-width, and double-ORDER findings are closed.

[P1] Use one total float order for both spill-run creation and k-way merging.

sortOrderedPercentileValues creates FLOAT32/FLOAT64 runs through mosort.SortByVectors, whose float comparators use ordinary < and > and do not order NaN. orderedPercentileRunHeap.Less merges those runs with cmp.Compare, which does order NaN. The merge therefore assumes an ordering that each input run does not satisfy. A supported NaN input can return a different percentile solely because the group crosses the spill threshold or lands on a different batch boundary.

Use the same explicit ASC/DESC total comparator in both paths, or reject NaN consistently, and add spill/non-spill equivalence coverage for FLOAT32/FLOAT64 and CONT/DISC.

1. 修复 FLOAT/DOUBLE NaN 在 percentile spill 下结果不稳定的问题

• 原因：spill run 创建时用 mosort.SortByVectors，FLOAT NaN 没有 total order；但 k-way merge 用 cmp.Compare，NaN 有 total order，导致 spill/non-spill 可能选到不同 rank。
• 修改：sortOrderedPercentileValues 改为直接使用 compareOrderedPercentileValue 排序。
• 结果：in-memory sort、spill run creation、k-way merge 三处使用同一套 ASC/DESC comparator。

2. 补充 NaN spill 等价测试

• 新增 FLOAT32 / FLOAT64 覆盖。
• 覆盖 ASC / DESC。
• 覆盖 PERCENTILE_CONT / PERCENTILE_DISC。
• 对比 spill 与 non-spill 结果一致。
• 额外断言 NaN 在 ASC/DESC 下的排序位置。

3. merge main 后修正 MORPC 协议版本冲突

• upstream main 已把 MORPCVersion16 用于 information_schema CHECK_CONSTRAINTS。
• percentile aggregate IDs 改为使用新的 MORPCVersion17。
• 远程执行门控改为 >= MORPCVersion17。
• 测试确认：Version16 仍拒绝 percentile remote，Version17 才允许。

4. 更新冲突 result 文件

• mo_cloud.result 接受 upstream 新增的 information_schema.check_constraints。
• information_schema 表数量从 24 改为 25。

Merge Queue Status

• ✅ Entered queue — 2026-08-11 11:22 UTC · Rule: main · triggered by rule Automatic queue on approval for main
• 🚫 Left the queue — 2026-08-11 11:40 UTC · at 4f620ec865b950d1b4e353217651518d28912234

This pull request spent 17 minutes 12 seconds in the queue, with no time running CI.

Reason

The pull request can't be updated

merge conflict between base and head

Hint

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Requeued — the merge queue status continues in this comment ↓.

Merge Queue Status

• ✅ Entered queue — 2026-08-11 13:34 UTC · Rule: main · triggered by rule Automatic queue on approval for main
• 🟠 Checks running · in-place
• 🚫 Left the queue — 2026-08-11 14:48 UTC · at 62ca3b7deb7a29ff59b3693348481b9bd77cac92

This pull request spent 1 hour 14 minutes 29 seconds in the queue, with no time running CI.

Reason

Pull request #26781 has been dequeued

Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.

The author needs to enable "Allow edits from maintainers" on this pull request.

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

• Requeue this pull request